Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add STARK batching #388

Draft
wants to merge 24 commits into
base: feat/continuations
Choose a base branch
from

Conversation

hratoanina
Copy link
Contributor

Replaces #366.

This one is targeting feat/continuations instead of develop.

@github-actions github-actions bot added the crate: evm_arithmetization Anything related to the evm_arithmetization crate. label Jul 11, 2024
@hratoanina hratoanina mentioned this pull request Jul 11, 2024
@Nashtare Nashtare added this to the zk-continuations - Q3 2024 milestone Jul 12, 2024
Copy link
Collaborator

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some perf comments. I suspect a large part of the overhead coming from the 3 sections cloning the entire PolyValues though

// Add lookup columns.
let lookups = stark.lookups();
let mut res = {
let mut columns = Vec::new();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let mut columns = Vec::new();
let mut columns = Vec::with_capacity(lookups.len() * config.num_challenges);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are more columns than that, since the number of lookup columns depend on the individual lookups. I don't think computing it just for the capacity is worth it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah it's a lower bound, but it avoids all intermediary allocations. Right now we're starting with a capacity of 4.

evm_arithmetization/src/prover.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/prover.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/prover.rs Outdated Show resolved Hide resolved
Comment on lines 536 to 537
let mut degree_bits_squashed = Table::all_degree_logs().to_vec();
degree_bits_squashed.dedup();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not a HashSet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a slice as argument for prove_openings so I decided to go with a vec directly.

evm_arithmetization/src/prover.rs Outdated Show resolved Hide resolved
evm_arithmetization/src/prover.rs Outdated Show resolved Hide resolved
Comment on lines 360 to 373
let trace_poly_values_sorted: [_; NUM_TABLES] = Table::all_sorted()
.iter()
.map(|&table| trace_poly_values[*table].clone())
.collect::<Vec<_>>()
.try_into()
.unwrap();

// We compute the Field Merkle Tree of all STARK traces.
let trace_polys_values_sorted_flat: Vec<_> = trace_poly_values_sorted
.clone()
.into_iter()
.flatten()
.collect();
let num_trace_polys = trace_polys_values_sorted_flat.len();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, the clones are way too heavy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These clones are a bit trickier to remove, but I got rid of the other two sections.

Comment on lines 90 to 97
pub fn zkevm_fast_config() -> StarkConfig {
let cap_height = 4;
let mut strategy = Vec::new();
for window in Table::all_degree_logs().windows(2) {
if window[0] != window[1] {
strategy.push(window[0] - window[1]);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you intend to have it work with dynamic batching? The config must be known for preprocessing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, good point. I guess the easiest way would be to add a BatchedStark strategy to compute this dynamically.

evm_arithmetization/src/all_stark.rs Outdated Show resolved Hide resolved
@Nashtare Nashtare force-pushed the stark_batching_continuations branch from 097e324 to f44f9a4 Compare July 29, 2024 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate: evm_arithmetization Anything related to the evm_arithmetization crate.
Projects
Status: Blocked
Development

Successfully merging this pull request may close these issues.

3 participants